Fix/fine-tune react-query settings #389

kwajiehao · 2021-03-25T03:51:07Z

This PR was reviewed with the staging branch of the backend.

Since changes are being made to the PageSettingsModal in PR #388, this PR should be rebased onto staging after #388 is merged into staging before being reviewed.

Overview

This PR makes 2 major updates to how we use the react-query library in our codebase:

~~Turning off automatic refetching behavior for~~ disable useQuery if changes have been made to local state
Performing query cache invalidation of the corresponding useQuery after a mutation

1. Turning off automatic refetching for Disable `useQuery` if changes to local state are made

Some of the components which we use useQuery in require us to maintain content state to track changes to the content that users have made (for example, in PageSettingsModal, we need to maintain local state to track changes to the page title that the users make).

However, currently, any changes made by the user are reset if the user navigates to another tab/window, and then returns to the CMS. This is reproducible through the following steps:

Go to the EditPage layout
Type some new content in the editor
Navigate to a different tab on your browser
Return to the CMS tab. The content that you typed would have disappeared

This is because react-query refetched the page content and overwrote the changes to local state. ~~Therefore, it's important to turn off refetching so we don't lose changes to local state made by the user.~~

We discussed this problem in depth, and the discussion minutes can be found in the CMS working document here (link). The conclusion was to keep a state variable, hasChanges, for detecting whether there are any changes to the local state from the initial mounted state. If there are changes, then hasChanges should be set to true, and we disable useQuery.

This solution is preferable because in most situations where we maintain local state and track local changes, we already check for changes in state to determine whether we should show a warning modal for users who attempt to navigate away with unsaved changes.

2. Performing query cache invalidation for corresponding `useQuery` after a mutation

Since the CMS needs to display the latest information to users, we need to ensure that our useQuery functions are not fetching outdated, cached data. Therefore, when we perform any mutations, we should invalidate the cache for the corresponding GET query so that we fetch fresh data the next time.

The alternative to this would be to set cacheTime to be 0 for all useQuery functions. Manual cache invalidation is more fine-grained and allows us to benefit from some degree of caching. Personally I'm open to either approach.

Misc. bugs fixed in this PR

This PR fixes a bug in parsing the directory file. Now that all base64 encoding and decoding occurs on the backend, there is no longer a need to base64 decode the directory file output before parsing it.
This PR standardizes the output of our GET API functions to return resp.data instead of just the response from the api call. resp.data is a better choice as we are able to use it in useEffect dependency arrays, whereas the resp object always changes, which will trigger the useEffect every time even if the object data hasn't actually changed.

gweiying · 2021-03-25T04:34:47Z

Some thoughts:

Disabling automatic refetching -

If our primary concern is that the changes are overwritten when a user navigates away from the page, how about instead setting a hasChanges flag, and doing passing { enabled: !hasChanges } to the query, rather than completely disabling refetching? Since one of the largest benefits of using the React Query library is that it automatically handles fetching, caching and updating data, turning off refetching seems to basically invalidate most of the benefits of the React Query library and not much different from a simple once-off API call on upon page load.

Performing query cache invalidation for corresponding useQuery after a mutation -

Makes sense. Should we create a separate file to import the queryClient object so that we don't have to instantiate a new object on every class?

kwajiehao · 2021-03-25T05:51:04Z

Some thoughts:

Disabling automatic refetching -

If our primary concern is that the changes are overwritten when a user navigates away from the page, how about instead setting a hasChanges flag, and doing passing { enabled: !hasChanges } to the query, rather than completely disabling refetching? Since one of the largest benefits of using the React Query library is that it automatically handles fetching, caching and updating data, turning off refetching seems to basically invalidate most of the benefits of the React Query library and not much different from a simple once-off API call on upon page load.

Is there any situation where we do want to have refetching for EditPages for example? The proposed changes affect only refetching so caching and other functionality are still maintained. For components where it makes sense, such as the Folders layout, we have allowed the default refetching settings.

~~Alternatively, we can set enabled to be false, and fetch the data manually using refetch~~

Updated description with conclusions from our meeting

Performing query cache invalidation for corresponding useQuery after a mutation -

Makes sense. Should we create a separate file to import the queryClient object so that we don't have to instantiate a new object on every class?

Sure, we can put it in a context and call the context - how does that sound?

kwajiehao · 2021-04-01T03:43:43Z

Performing query cache invalidation for corresponding useQuery after a mutation -

Makes sense. Should we create a separate file to import the queryClient object so that we don't have to instantiate a new object on every class?

@gweiying upon further research, useQueryClient returns the current queryClient instance so we're not actually creating a new object (useQueryClient documentation)

alexanderleegs

lgtm!

Outdated

This commit sets the `refetchOnWindowFocus` flag for the useQuery on the following layouts/components to be `false`: - PageSettingsModal - EditNavBar - EditPage The reason we turn off this flag is because these pages involve user changes - with this flag on, any unsaved changes by the user would be overwritten by the refetched data. This commit also sets the - `refetchOnReconnect` - `refetchInterval` - `refetchIntervalInBackground` flags to be false for the same reasons explained above.

This commit adds cache invalidation of all our GET queries (the useQuery invocations) after we perform a mutation. This allows us to reset our cache in a more granular manner, as opposed to simply setting the cache time for our GET queries to be 0. Additionally, this commit also adds a PAGE_SETTINGS_KEY constant to replace the string literal that was being used as the query key for the PageSettingsModal. Refer to the following documentation for more details: - https://react-query.tanstack.com/guides/query-invalidation - https://react-query.tanstack.com/guides/invalidations-from-mutations

This commit standardizes the output of our GET API functions to return resp.data instead of just the response from the api call. resp.data is a better choice as we are able to use that in dependency arrays, whereas the resp object pointer always changes, which will trigger the useEffect every time even if the object data hasn't actually changed.

As per our discussion (refer to meeting minutes here: https://docs.google.com/document/d/1br6T6wVX0KrcA3nwQEo7OhUrcT4veLnaz0vByEXjVvo/edit#heading=h.hyx8t36v9z3n) we will be discussing our `useQuery` functions if there are changes to the local state that are being tracked, to avoid refetching behavior from overwriting local changes.

…ata directly

…or CollectionPagesSection

gweiying · 2021-04-29T09:23:36Z

src/components/FolderModal.jsx

+      onSuccess: () => {
+        if (!isCollection) {
+          // Resource folder
+          console.log('hey')


gweiying · 2021-04-29T09:24:50Z

src/layouts/Folders.jsx

@@ -128,6 +131,10 @@ const Folders = ({ match, location }) => {
          setSelectedPage('')
          errorToast(`The page data could not be retrieved. ${DEFAULT_RETRY_MSG}`)
        },
+        onSuccess: () => {
+          queryClient.invalidateQueries([DIR_CONTENT_KEY, siteName, folderName])


to add successToast

gweiying

lgtm! thanks for making all the adjustments

* fix: turn off refetching on window focus for useQuery This commit sets the `refetchOnWindowFocus` flag for the useQuery on the following layouts/components to be `false`: - PageSettingsModal - EditNavBar - EditPage The reason we turn off this flag is because these pages involve user changes - with this flag on, any unsaved changes by the user would be overwritten by the refetched data. This commit also sets the - `refetchOnReconnect` - `refetchInterval` - `refetchIntervalInBackground` flags to be false for the same reasons explained above. * fix: invalidate queries after mutation This commit adds cache invalidation of all our GET queries (the useQuery invocations) after we perform a mutation. This allows us to reset our cache in a more granular manner, as opposed to simply setting the cache time for our GET queries to be 0. Additionally, this commit also adds a PAGE_SETTINGS_KEY constant to replace the string literal that was being used as the query key for the PageSettingsModal. Refer to the following documentation for more details: - https://react-query.tanstack.com/guides/query-invalidation - https://react-query.tanstack.com/guides/invalidations-from-mutations * feat: standardize output of GET API calls to return resp.data This commit standardizes the output of our GET API functions to return resp.data instead of just the response from the api call. resp.data is a better choice as we are able to use that in dependency arrays, whereas the resp object pointer always changes, which will trigger the useEffect every time even if the object data hasn't actually changed. * fix: load yaml content when reading directory file * feat: disable useQuery if component tracks local state As per our discussion (refer to meeting minutes here: https://docs.google.com/document/d/1br6T6wVX0KrcA3nwQEo7OhUrcT4veLnaz0vByEXjVvo/edit#heading=h.hyx8t36v9z3n) we will be discussing our `useQuery` functions if there are changes to the local state that are being tracked, to avoid refetching behavior from overwriting local changes. * fix: rebase errors * fix: invocation of LoginContext * Fix: rebase errors * Fix: update resource category and resource page get calls to return data directly * Refactor: use invalidateQueries instead of passing refetch function for CollectionPagesSection * Fix: error when retrieving page settings * Fix: rebase errors * Fix: invalidate query instead of reload when changing page settings * Feat: add success toast when changing settings * Fix: change folder naming to use invalidation instead of reload * Fix: invalidate correct resource folder key * Fix: update success toast messages * Fix: remove log statement * Fix: add successtoast Co-authored-by: Alexander Lee <[email protected]>

* Add linting and formatting tools (#378) * fix: outdated packages with vulnerabilities * feat: install eslint and initiate config * feat: install prettier and set prettier options * feat: install eslint-config-prettier * feat: install eslint-plugin-prettier * chore: reformat eslint config * feat: add @trivago/prettier-plugin-sort-imports, define preferred import order * fix: css-loader file resolution bug introduced by CRA v4 In recent commits, we upgraded our react-scripts version from 3.4.4 to 4.0.3. This is because CRA (create-react-app) v3 uses an outdated version of eslint (facebook/create-react-app#8849). This introduced a bug related to the css-loader library, which can no longer resolve assets in the public folder: - facebook/create-react-app#9870 (comment) - webpack-contrib/css-loader#1136 (comment) This commit fixes this bug by moving the referenced image to the relevant sub-directory in the src directory. * chore: temporarily disable eslint * chore: add more files and folders to .prettierignore * chore: upgrade prettier-plugin-sort-imports to 2.0.2 fixes trivago/prettier-plugin-sort-imports#22 * chore: temporarily disable prettier * chore: remove prettier config temporarily * chore: remove jsx-a11y references temporarily * temporarily remove import/prefer-default-export reference Co-authored-by: jiehao <[email protected]> Co-authored-by: Preston Lim <[email protected]> * Fix/resource color (#430) * fix: resource page header changed to bg-secondary * fix: using isResourcePage to determine page header * Fix/rearrange layout (#427) * fix: simplify directoryFile utils for retrieve and update * fix: update methods using directoryFile utils * fix: introduce FolderReorderingModal * fix: refactors FolderContent * fix: update params for FolderContentItem * fix: fix breadcrumb display * fix: add propTypes for FolderReorderingModal * fix: add Cancel button to FolderReorderingModal * fix: updates draggable-id for React dnd * fix: updates dropdown button behavior for reordering * fix: updates copy text * fix: updates copy text * fix: updates variable naming for directory file output * refactor: clean up ProtectedRoute and LoginContext (#431) * refactor: clean up ProtectedRoute and LoginContext * refactor: make LoginContext testable create 3 exports: LoginContext itself, LoginProvider, LoginConsumer * refactor: make route selector testable in App.jsx * refactor: group routing components * feat: add basic routing tests * refactor: move __tests__ folder to correct place Required for jest to find the test files * style: delete unnecessary div * refactor: make exports more obvious for LoginContext.js * refactor: delete duplicate route * chore: add rest of routing tests * style: remove unused declarations * Fix/fine-tune react-query settings (#389) * fix: turn off refetching on window focus for useQuery This commit sets the `refetchOnWindowFocus` flag for the useQuery on the following layouts/components to be `false`: - PageSettingsModal - EditNavBar - EditPage The reason we turn off this flag is because these pages involve user changes - with this flag on, any unsaved changes by the user would be overwritten by the refetched data. This commit also sets the - `refetchOnReconnect` - `refetchInterval` - `refetchIntervalInBackground` flags to be false for the same reasons explained above. * fix: invalidate queries after mutation This commit adds cache invalidation of all our GET queries (the useQuery invocations) after we perform a mutation. This allows us to reset our cache in a more granular manner, as opposed to simply setting the cache time for our GET queries to be 0. Additionally, this commit also adds a PAGE_SETTINGS_KEY constant to replace the string literal that was being used as the query key for the PageSettingsModal. Refer to the following documentation for more details: - https://react-query.tanstack.com/guides/query-invalidation - https://react-query.tanstack.com/guides/invalidations-from-mutations * feat: standardize output of GET API calls to return resp.data This commit standardizes the output of our GET API functions to return resp.data instead of just the response from the api call. resp.data is a better choice as we are able to use that in dependency arrays, whereas the resp object pointer always changes, which will trigger the useEffect every time even if the object data hasn't actually changed. * fix: load yaml content when reading directory file * feat: disable useQuery if component tracks local state As per our discussion (refer to meeting minutes here: https://docs.google.com/document/d/1br6T6wVX0KrcA3nwQEo7OhUrcT4veLnaz0vByEXjVvo/edit#heading=h.hyx8t36v9z3n) we will be discussing our `useQuery` functions if there are changes to the local state that are being tracked, to avoid refetching behavior from overwriting local changes. * fix: rebase errors * fix: invocation of LoginContext * Fix: rebase errors * Fix: update resource category and resource page get calls to return data directly * Refactor: use invalidateQueries instead of passing refetch function for CollectionPagesSection * Fix: error when retrieving page settings * Fix: rebase errors * Fix: invalidate query instead of reload when changing page settings * Feat: add success toast when changing settings * Fix: change folder naming to use invalidation instead of reload * Fix: invalidate correct resource folder key * Fix: update success toast messages * Fix: remove log statement * Fix: add successtoast Co-authored-by: Alexander Lee <[email protected]> * fix: pass parameters to wrapped components (#439) * fix: fixes toast popup on item select, folder deletion modal (#440) * fix: fixes toast popup on item select, folder deletion modal * fix: add condition for folder deletion Co-authored-by: kwajiehao <[email protected]> Co-authored-by: jiehao <[email protected]> Co-authored-by: Preston Lim <[email protected]> Co-authored-by: Alexander Lee <[email protected]> Co-authored-by: Alexander Lee <[email protected]>

kwajiehao requested review from alexanderleegs and gweiying March 25, 2021 03:51

kwajiehao force-pushed the fix/misc-issues branch 2 times, most recently from b682342 to 339f23b Compare April 1, 2021 03:30

kwajiehao requested review from alexanderleegs and gweiying and removed request for alexanderleegs and gweiying April 1, 2021 03:43

alexanderleegs previously approved these changes Apr 7, 2021

View reviewed changes

prestonlimlianjie force-pushed the staging branch 2 times, most recently from 97243c5 to 104585d Compare April 22, 2021 08:56

gweiying force-pushed the staging branch 2 times, most recently from 789315a to 3edf305 Compare April 22, 2021 10:01

prestonlimlianjie force-pushed the staging branch 4 times, most recently from 473f913 to 104585d Compare April 22, 2021 16:19

gweiying force-pushed the staging branch from 2aa238b to a217464 Compare April 26, 2021 02:36

gweiying changed the base branch from staging to develop April 28, 2021 08:18

alexanderleegs force-pushed the fix/misc-issues branch from 583c455 to 71823a4 Compare April 28, 2021 10:08

kwajiehao added 6 commits April 29, 2021 15:26

fix: load yaml content when reading directory file

2d85034

fix: rebase errors

6ff3c4f

kwajiehao and others added 6 commits April 29, 2021 15:29

fix: invocation of LoginContext

21bb7f4

Fix: rebase errors

4893f59

Fix: update resource category and resource page get calls to return d…

7865561

…ata directly

Refactor: use invalidateQueries instead of passing refetch function f…

8a7b14b

…or CollectionPagesSection

Fix: error when retrieving page settings

d58c728

Fix: rebase errors

61ea027

alexanderleegs force-pushed the fix/misc-issues branch from 37cb353 to 61ea027 Compare April 29, 2021 07:47

alexanderleegs added 3 commits April 29, 2021 16:21

Fix: invalidate query instead of reload when changing page settings

08e7421

Feat: add success toast when changing settings

2ad447a

Fix: change folder naming to use invalidation instead of reload

b38a98e

alexanderleegs mentioned this pull request Apr 29, 2021

Add query invalidation abstraction #437

Open

alexanderleegs added 2 commits April 29, 2021 17:09

Fix: invalidate correct resource folder key

f34f763

Fix: update success toast messages

2a1852f

gweiying reviewed Apr 29, 2021

View reviewed changes

alexanderleegs added 2 commits April 29, 2021 17:25

Fix: remove log statement

6ad1ba3

Fix: add successtoast

ba2f930

gweiying approved these changes Apr 29, 2021

View reviewed changes

alexanderleegs merged commit 4576ab0 into develop Apr 29, 2021

alexanderleegs deleted the fix/misc-issues branch April 29, 2021 09:32

gweiying mentioned this pull request Apr 30, 2021

(Merge to prod) 30 April #436

Merged

snyk-bot mentioned this pull request Jan 17, 2022

[Snyk] Fix for 2 vulnerabilities #730

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/fine-tune react-query settings #389

Fix/fine-tune react-query settings #389

kwajiehao commented Mar 25, 2021 •

edited

Loading

gweiying commented Mar 25, 2021

kwajiehao commented Mar 25, 2021 •

edited

Loading

kwajiehao commented Apr 1, 2021

alexanderleegs left a comment

gweiying Apr 29, 2021

alexanderleegs Apr 29, 2021

gweiying Apr 29, 2021 •

edited

Loading

alexanderleegs Apr 29, 2021

gweiying left a comment

Fix/fine-tune react-query settings #389

Fix/fine-tune react-query settings #389

Conversation

kwajiehao commented Mar 25, 2021 • edited Loading

Overview

1. Turning off automatic refetching for Disable useQuery if changes to local state are made

2. Performing query cache invalidation for corresponding useQuery after a mutation

Misc. bugs fixed in this PR

gweiying commented Mar 25, 2021

kwajiehao commented Mar 25, 2021 • edited Loading

kwajiehao commented Apr 1, 2021

alexanderleegs left a comment

Choose a reason for hiding this comment

gweiying Apr 29, 2021

Choose a reason for hiding this comment

alexanderleegs Apr 29, 2021

Choose a reason for hiding this comment

gweiying Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

alexanderleegs Apr 29, 2021

Choose a reason for hiding this comment

gweiying left a comment

Choose a reason for hiding this comment

kwajiehao commented Mar 25, 2021 •

edited

Loading

1. Turning off automatic refetching for Disable `useQuery` if changes to local state are made

2. Performing query cache invalidation for corresponding `useQuery` after a mutation

kwajiehao commented Mar 25, 2021 •

edited

Loading

gweiying Apr 29, 2021 •

edited

Loading